Hey there @PhracturedBlue, @tetienne, mind taking a look at this pull request as its been labeled with an integration (template) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

This means that as a user with Imperial units in nearly all my current entities, I now need to convert them to be in Metric units to use this template? Then the template entities will convert them back? I don't see the benefit here. I think it would be easier and more straightforward to document the units expected in the documentation.

This means that as a user with Imperial units in nearly all my current entities, I now need to convert them to be in Metric units to use this template? Then the template entities will convert them back? I don't see the benefit here. I think it would be easier and more straightforward to document the units expected in the documentation.

Have you read this comment? As far as I can see, there is no other way if users shall be able to create weather templates which work with Metric as well as Imperial unit system. Note that I'm relatively new to HA - I stand ready to be corrected... As a side note: I don't care whether templates shall return Metric or Imperial values (as long as the unit is fixed).

I agree that users cannot make a template that can convert back and forth from Metric and Imperial, but I don't think this will be a very common use. Users stay in one unit system I would think, and only need to set up units once in this template.

Another issue is that this PR only converts the current windspeed, for example, but not windspeed in the forecasts. So in Imperial units, the windspeed would have to be supplied in two different units.

I agree that this is not a common use case, but this solution has another advantage: It is more transparent. I had to figure out what units to return on my own, and had to understand that I just had to compute the values as they fit and ignore the problem with different time systems (I found that confusing). I also want to point you to the fact that WeatherTemplate is a subclass of WeatherEntity, and that class requires the behavior I have implemented: the docs say that Properties have to follow the units defined in the unit_system, which I take as "have to return values in units as configured in hass.config.units". Thus, WeatherTemplate should behave the same way.

Concerning the forecast issue: The main use case here is that the forecast is reused by another weather entity (as I did in my example configuration above: forecast_template: "{{ states.weather.my_region.attributes.forecast }}"), so in that case it's not the templates' responsibility. And in fact, at least WeatherBit, OpenWeatherMap and DWD do provide the correct values with respect to the configured unit system (at least for temperature), so that point is more in my favor, I guess ;-) I do agree, though, that in case I want to compute an own forecast, I'm again lost, since I cannot figure out the configured unit system in the template code. Maybe that information should be added to the docs of forecast_template!?

the docs say that Properties have to follow the units defined in the unit_system, which I take as "have to return values in units as configured in hass.config.units"

This could be satisfied by putting the required units of both systems into the user documentation. Just my opinion, and you should wait for other opinions.

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

The bot and I are still waiting for other opinions...

I found this pr by luck after trying to make the ecobee integration compatible with both metric & imperial, and have a few thoughts:

• Ideally, all weather integrations should be converting their outputs to the correct units. I'm not sure about sensors; maybe they aren't converted? Anyways, if weather integration values were used in a weather template expecting fixed units, the output would break. So I don't think fixed units in the weather template makes sense.
• There are only two weather integrations as of now converting to correct units. All the rest seem to be using fixed and undocumented (and likely inconsistent) units.
• Converting units inside every single weather component sounds like a mess, and your code for templates but instead for the weather integration might really help. Then integrations could export to some fixed units, and then the base weather integration would convert to the correct unit, and templates would use the localized units.
• An even better solution might be to use the same scheme as temperature, where there would be an extra pressure_units property. This could be on both the templates and weather integration. This property can be used to determine whether the template/integration outputs already localized units or fixed ones (i.e. if not implemented, the units are localized. If implemented, the units are whatever pressure_units returns). And then do the conversion as you are doing in your pr...

I'm not a maintainer here so this is not authoritative advice at all. Just hope it helps 😁

• Ideally, all weather integrations should be converting their outputs to the correct units. I'm not sure about sensors; maybe they aren't converted? Anyways, if weather integration values were used in a weather template expecting fixed units, the output would break. So I don't think fixed units in the weather template makes sense.

That's probably true, but I still don't see any alternative since I don't see a way to convert units in a weather template (because it's just not possible to know what the correct unit would be).

• There are only two weather integrations as of now converting to correct units. All the rest seem to be using fixed and undocumented (and likely inconsistent) units.

There might be a few more (I remember some weather integrations showing the correct units quite some time after changing the unit system through the UI), but I agree that this is dealt with in an inconsistent manner in many places (and that's clearly a sign for lack of documentation etc).

• Converting units inside every single weather component sounds like a mess, and your code for templates but instead for the weather integration might really help. Then integrations could export to some fixed units, and then the base weather integration would convert to the correct unit, and templates would use the localized units.
• An even better solution might be to use the same scheme as temperature, where there would be an extra pressure_units property. This could be on both the templates and weather integration. This property can be used to determine whether the template/integration outputs already localized units or fixed ones (i.e. if not implemented, the units are localized. If implemented, the units are whatever pressure_units returns). And then do the conversion as you are doing in your pr...

This might indeed be an option. An even simpler (and for my feeling more clear) approach would be to have HA internally work with fixed units (e.g., °C for temperature), and only make the conversion to the localized unit at the UI level. However, that also comes with drawbacks (e.g., what units do I rely on in my automations? Also going for the internal unit (°C) would make it more difficult to write automations, and relying on the localized unit would break automations if the unit system is changed). Thus, I think that the latter (providing explicit units) might indeed be the best option. In fact, again I think that the existence of temperature_unit shows once more that this problem is not new (but has so far only been tackled for temperature). However, this is going to break lots of stuff, I would guess, so let's see whether something like this happens :-)

Thus, I think that the latter (providing explicit units) might indeed be the best option.

Agree. Making the additional ░░░░_unit properties optional might be a way to introduce the change without breaking anything. It just might take many changes to the codebase though.

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@csoltenborn are you still working on this? If you don't have the time, I'd be interested in picking up the work.

Well, I'm kind of waiting for more feedback - not sure what could be done at the moment. What do you have in mind?

@frenck Maybe this PR is also stalled because of wrong codeowner.

Well, I'm kind of waiting for more feedback - not sure what could be done at the moment.

I'd love to hear some too 😁

What do you have in mind?

Perhaps this is better to leave until a maintainer gives feedback, but it would be nice to allow the author to specify which units they are using in the template.

I'd also like to adapt the code in the PR to weather integrations. Is this something you would like to work on, or I could make my own PR (double the exposure!)

It seems that the bot tagged the wrong people. That may explain why this PR has less exposure than you expect. AFAIU from commits and comments @emontnemery is the code owner of the template integration.

@FrnchFrgg Thanks for pointing this out - in fact, I thought about closing this PR after the 2nd bot complaint because I felt kind of ignored :-)

@rianadon From the discussions, it appeared to me that "unit issues" are deeply integrated into HA, and that there is no simple way to fix this (since a "real" fix would break lots of integrations). What do you have in mind with "adapt PR to weather integrations"?

@csoltenborn
The output of the weather entity should be according the unit system as configured by the user, except for the temperature unit which can be overridden.

Instead of forcing the output of the templates to be one or the other, and then converting back, we could make the expected unit available as a template variable, so we would pass unit_of_measurement=hPa or unit_of_measurement=inHg to the pressure template. This would be a non-breaking change, but it would still be possible to write advanced templates which are not just copying a sensor state.

Documentation

The existing documentation is vague to say the least and needs to be updated to make that clear.
I think your PR home-assistant/home-assistant.io#17241 is a good start, could you update it to reflect the current situation (pressure is in hPa or inHg according to the user's configuration and so on)?

Sensor units

Assuming that the input to the templates are sensor states, they should be automatically converted to the unit system configured by the user.
It's a bug if the sensor state is not converted automatically, but it's a very common bug affecting a lot of integrations.

There's a (currently stalled) effort underway to move unit conversion to the sensor base class so it doesn't have to be implemented in every sensor: #48261.

This looks awesome @emontnemery! Your PR is exactly what I was hoping for.

Hey, there's some feedback! :-) Thanks, @emontnemery !

@rianadon I'm not going to have time for this in the coming weeks - would you indeed like to pick up my work?

I'll see what I can do!

Can do! My plan as of now is:

1. Add pressure_unit, wind_speed_unit, visibility_unit as additional properties to WeatherTemplate
2. Add these three properties to WeatherEntity too (so WeatherTemplate overrides them). Their default implementations will return the user-configured unit system so nothing breaks.
3. Expect the three properties plus temperature_unit to be optionally set in the template config
4. Set the three properties plus temperature_unit to whatever they are set to in the config; if they are not set, set them to the user-configured unit system.
5. Move the unit conversion to the state_attributes method of WeatherEntity (where the temperature unit conversion is right now). pressure, wind_speed, and visibility methods will return to how they were before (i.e. they return the value from the template, without any unit conversion)

Alternatively, I could keep my changes smaller by only implementing # 3, then doing unit conversion in WeatherTemplatelike @csoltenborn has implemented. However, if the weather entity units are to be fixed as well, the first plan will refactor so these unit conversions are only being done in one place.

@emontnemery unfortunately WeatherTemplate does not extend SensorEntity, so I cannot build off any of your work. What do you think about moving unit conversion up to the Entity class, so WeatherTemplate can use it too?

Things I'm not sure on:

• Should self.add_template_attribute be called with the unit fields (pressure_unit, etc)?
• How to deal with precision?

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@rianadon, @csoltenborn do you plan to pick this up?

Hey. Sorry I've gottten busy lately. I was waiting for #53846 to get merged, and I haven't had the chance yet to make the revisions you requested to that PR. I do plan on picking this up but it'll likely be several weeks until I get to it.

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.